fix: resolve MCP server execution on Windows with node version managers (#1246) #4711
Conversation
dosubot
bot
added
size:L
hannesrudolph
left a comment
hannesrudolph
left a comment
There was a problem hiding this comment.
Thanks for this fix! The implementation is clean and well-tested. The approach of wrapping commands with cmd.exe /c on Windows is a pragmatic solution that should resolve the immediate issue with node version managers.
Summary
This PR successfully addresses the "spawn npx ENOENT" error on Windows by ensuring commands are executed through the Windows command interpreter, which can properly handle PowerShell scripts and other non-executable commands.
Strengths
✅ Minimal, focused change that only affects Windows platforms
✅ Comprehensive test coverage including platform-specific behavior
✅ Maintains backward compatibility for non-Windows systems
✅ Clear implementation that's easy to understand and maintain
Questions for Consideration
I've left a few inline comments about:
- The decision to diverge from the originally suggested approach (environment variables + shell: true)
- Potential edge cases with command double-wrapping
- Performance implications for long-running servers
- Opportunities for additional test coverage
Overall, this is a solid fix that should resolve the immediate issue. The code quality is good and the tests give confidence that the change won't break existing functionality.
| ) | ||
|
|
||
| // Initialize servers (this will trigger connectToServer) | ||
| await mcpHub["initializeGlobalMcpServers"]() |
There was a problem hiding this comment.
Great test coverage! The tests properly verify the Windows command wrapping behavior and ensure non-Windows platforms remain unaffected.
One suggestion: Would it be valuable to add a test case that verifies the fix actually resolves the original issue - perhaps a test that simulates the npx.ps1 scenario mentioned in the issue description?
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
left a comment
hannesrudolph
left a comment
There was a problem hiding this comment.
Thanks for the thorough review! I've addressed all your feedback:
- Environment variables: The implementation already passes all environment variables through properly
- Double-wrapping prevention: Added checks to avoid wrapping commands that are already cmd.exe
- Test coverage: Added comprehensive tests including the npx.ps1 scenario
- Performance documentation: Added comments explaining the overhead
All tests are passing and the changes have been pushed.
| ) | ||
|
|
||
| // Initialize servers (this will trigger connectToServer) | ||
| await mcpHub["initializeGlobalMcpServers"]() |
There was a problem hiding this comment.
✅ Added - I've added comprehensive test coverage including:
- Test for preventing double-wrapping of cmd.exe commands
- Test for case-insensitive cmd command checking
- Test specifically simulating the npx.ps1 scenario from node version managers
All tests are passing (21/21).
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
daniel-lxs
left a comment
daniel-lxs
left a comment
There was a problem hiding this comment.
This looks okay, but I'm not sure if it's the ideal solution.
It might be worth exploring some alternatives.
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Needs Review]
in Roo Code Roadmap
|
Mind addressing the conflict? Thank you! |
…rs (#1246) - Wrap commands with cmd.exe /c on Windows to handle non-exe executables - Add platform detection for Windows-specific command handling - Add comprehensive tests for Windows command wrapping behavior - Maintain backward compatibility for non-Windows platforms This fixes the 'spawn npx ENOENT' error that occurs when using node version managers like fnm on Windows, where npx is a PowerShell script rather than an executable file.
daniel-lxs
force-pushed
the
fix/issue-1246-windows-mcp-npx
branch
from
00996bc to
c9e60a8
Compare
github-project-automation
bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
…rs (RooCodeInc#1246) (RooCodeInc#4711) Co-authored-by: Daniel Riccio <[email protected]>
…rs (#1246) (#4711) Co-authored-by: Daniel Riccio <[email protected]>
AUTOMATED PR CREATION
Description
Fixes #1246
This PR resolves the "spawn npx ENOENT" error that occurs when using MCP servers on Windows with node version managers like fnm, where
npxis implemented as a PowerShell script rather than an executable file.Changes Made
McpHub.tsto wrap commands withcmd.exe /cconnectToServermethod to handle non-executable commands on WindowsTesting
Verification of Acceptance Criteria
Checklist
Technical Details
The issue occurs because node version managers on Windows often implement
npxas a PowerShell script (e.g.,npx.ps1) rather than as an executable file. When Node.js tries to spawn this directly, it fails with ENOENT because it's not a valid executable.By wrapping the command with
cmd.exe /con Windows, we ensure that the Windows command interpreter handles the execution, which properly resolves and runs PowerShell scripts and other non-executable commands.